fix: copy_dir and copy_file to preserve symlinks instead of following them.
#4671
Conversation
Previously, copy_dir would follow symlinks and copy the target contents. Now it preserves symlinks by reading the link target and creating a new symlink with the same target. Adds regression test copy_dir_preserves_symlinks.
Previously, copy_file created a symlink pointing to the source path rather than preserving the original symlink's target. Now it reads the link target and creates a symlink with that same target. Adds regression test copy_file_preserves_symlinks.
cachebag
force-pushed
the
preserve-symlinks
branch
from
7ce58b5 to
5460b37
Compare
|
Thanks for making this patch! Reasonable support for external packagers are still expected, and whether a manual setup is required is largely based on the packagers' opinion: https://rust-lang.github.io/rustup/installation/other.html#using-a-package-manager I have the feeling that we need to somehow support both cases based on whether that's copying the original binary or not... |
There was a problem hiding this comment.
Do you have a specific use case where rustup is not working as you'd expect, which this PR allows to fix?
If you can find such a case, I agree with using this new implementating except for this:
Line 845 in 3382f6f
#1521 is probably too widespread as a fix, and it makes perfect sense to use different copy implementations during self installation and during toolchain installation.
Suggest refining the test cases accordingly if going this way.
| let dest = dest.join(entry.file_name()); | ||
| if kind.is_dir() { | ||
| copy_dir(&src, &dest)?; | ||
| let src_path = entry.path(); |
There was a problem hiding this comment.
Nit: src and dest bindings in the loop body are preventing the input params from being used. Thus, I don't see a very convincing reason to rename them here.
2 participants
This PR employs symlink preservation in
copy_fileand adds it tocopy_dir.PR #1504 added symlink preservation to fix crashes in lldb-preview, which contains internal symlinks (liblldb was loading twice due to broken symlinks). Then, PR #1521 later changed
copy_fileto create symlinks pointing to the source path (for Homebrew integration), which may have regressed #1504.This PR restores #1504's behavior: read the symlink target and preserve it. What I'm curious about is what the intended behavior for #1521 was because shouldn't users be able to run
rustup-init --no-self-update? Is that not the recommended way to managerustupvia external package managers?If this PR steers out of scope of the goals of
rustupI'm happy to close it but I feel like the former behavior is preferable for most use cases.